Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.5] Eloquent model without updated_at field #21178

Merged
merged 1 commit into from
Sep 14, 2017
Merged

[5.5] Eloquent model without updated_at field #21178

merged 1 commit into from
Sep 14, 2017

Conversation

linaspasv
Copy link
Contributor

Since Laravel 5.0 many found a simple method to have eloquent models without updated_at field. Unfortunatelly this is not a thing in Laravel 5.5 anymore ( #19627 #21045 #20901 #20930 ...)

e.g. how it was used before Laravel 5.5

class MyModel extends Model
{
    const UPDATED_AT = null;
}

I do understand it's not an official method but since we do lack flexibility on the subject, please, leave this behaviour as it was working before...

Copy link
Contributor

@antonkomarev antonkomarev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need nested conditional statement when && could be used?

if (static::UPDATED_AT !== null && ! $this->isDirty(static::UPDATED_AT)) {
    $this->setUpdatedAt($time);
}

@themsaid
Copy link
Member

This was proposed in #20930 and rejected.

Just referencing.

@linaspasv
Copy link
Contributor Author

@themsaid correct but I see too many people, including me, were using like so before the Laravel 5.5. This PR doesn't add any new behaviour like proposed in #20930 , just brings back the use-case of Laravel 5.0-5.4 for the time being.

@taylorotwell taylorotwell merged commit b4758e2 into laravel:5.5 Sep 14, 2017
@linaspasv
Copy link
Contributor Author

@taylorotwell thanks!

@segadora
Copy link
Contributor

This will conflict with SoftDeletes

if ($this->timestamps) {
$this->{$this->getUpdatedAtColumn()} = $time;
$columns[$this->getUpdatedAtColumn()] = $this->fromDateTime($time);

It should also be supported there.

@salarmehr
Copy link

setting CREATED_AT and UPDATED_AT constants to false could be enough for achieving a more granular result of the nasty redundant $timestamps property in the first stage.

@linaspasv
Copy link
Contributor Author

linaspasv commented Sep 20, 2017

@segadora thank you for spotting this!

@salarmehr many are using null for UPDATED_AT, so maybe empty check could cover both cases (otherwise it's a breaking change), you might try to file a PR for letting use false too. I do agree false might feel a bit more natural.

@salarmehr
Copy link

salarmehr commented Sep 20, 2017

@segadora Thanks for your comment. My comment mainly refers to the defect of the Model class itself.

@antonkomarev
Copy link
Contributor

@linaspasv As I remember I've tried to change this value to false and got same result.

@segadora
Copy link
Contributor

@linaspasv I will agree with a empty check.

@pdbreen
Copy link

pdbreen commented Sep 29, 2017

Unfortunately, I've got a legacy table that does have an UPDATED_AT, but does not have CREATED_AT column. This could be done prior to 5.5 by setting CREATED_AT = null.

Looks like I'll need to turn off timestamps entirely and manage the update time myself in 5.5

@antonkomarev
Copy link
Contributor

@pdbreen it would be better to turn off timestamps for such case and manage them manually as you proposed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants